Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Adding more dedicated exceptions #739

Open
wants to merge 1 commit into
base: master
Choose a base branch
from

Conversation

ThomasLandauer
Copy link

@ThomasLandauer ThomasLandauer commented Oct 2, 2024

Type of pull request

  • Bug fix (involves code and configuration changes)
  • New feature (involves code and configuration changes)
  • Documentation update
  • Something else

About

Adding 2 dedicated exceptions, following the path of #433

2 remarks:

Checklist for code / configuration changes

In case you changed the code/configuration, please read each of the following checkboxes as they contain valuable information:

  • Please add at least one test case (unit test, system test, ...) to demonstrate that the change is working. If existing code was changed, your tests cover these code parts as well.
    By the way, you don't have to provide a full fledged PDF file to demonstrate a fix. Instead a unit test may be sufficient sometimes,
    please have a look at FontTest for example code.
    Code changes without any tests are likely to be rejected. If you dont know how to write tests, no problem, tell us upfront and we may add them ourselves or discuss other ways.
  • Please run PHP-CS-Fixer before committing, to confirm with our coding styles. See https://github.com/smalot/pdfparser/blob/master/.php-cs-fixer.php for more information about our coding styles.
  • In case you fix an existing issue, please do one of the following:
    • Write in this text something like fixes #1234 to outline that you are providing a fix for the issue #1234.
    • After the pull request was created, you will find on the right side a section called Development. There issues can be selected which will be closed after the your pull request got merged.
  • In case you changed internal behavior or functionality, please check our documentation to make sure these changes are documented properly: https://github.com/smalot/pdfparser/tree/master/doc
  • In case you want to discuss new ideas/changes and you are not sure, just create a pull request and mark it as a draft
    (see here for more information).
    This will tell us, that it is not ready for merge, but you want to discuss certain issues.

@k00ni
Copy link
Collaborator

k00ni commented Oct 3, 2024

👍 I like your idea of dedicated exceptions. While we are at it, can you think of other exception types?

The remarks are also important, but I would like to discuss these in a separate issue/pull request. A dedicated CONTRIBUTING.md file seems to be a good idea. Would you mind suggesting one in a separate PR?

@ThomasLandauer
Copy link
Author

While we are at it, can you think of other exception types?

Fulltext search for \Exception yields some hits. Do you want me to go through them and create exceptions? (But I'd just take the current exception message and create a class from it - I won't research the context) Or just leave it up to others who have a real need (and know why/when it occurs)?

A dedicated CONTRIBUTING.md file seems to be a good idea. Would you mind suggesting one in a separate PR?_

I'm a beginner with git, so I'm probably the wrong person. As a starting point, you can copy over what you currently have. I was annoyed yesterday to find your checklist only after I had finished my PR - so the idea is just to tell people upfront :-)

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
None yet
Development

Successfully merging this pull request may close these issues.

2 participants